fix: windows compilation of hash maps tests and increase stack size avoiding segfaults #988
Conversation
jalvesz
changed the title
|
Ideally there would be a way to avoid the issues with the libs, instead of fixing them this way, but it seems to work. |
|
Also this may warrant adding the intel oneapi llvm compiler on windows to the compilers which work, but aren't tested in the readme. |
|
Yes, I agree that ideally this should be managed differently but I'm not aware of a better solution compared to what we found here. It is good that indeed, it enables full compliance on Windows for the tests. |
| @@ -2,6 +2,14 @@ if (NOT TARGET "test-drive::test-drive") | |||
| find_package("test-drive" REQUIRED) | |||
| endif() | |||
|
|
|||
| if(WIN32) | |||
| if(CMAKE_Fortran_COMPILER_ID MATCHES "^Intel") | |||
| add_link_options(/Qoption,link,/STACK:8388608) | |||
There was a problem hiding this comment.
I've looked in the hashmap source file and I can't find reasons more stack size would be needed: there are no static (save)d variables or arrays there, only allocatables.
I'm worried that increasing the stack size may just be hiding other issues with the code that we haven't figured out yet. For example, the test cases include static arrays, maybe they could be replaced with allocatables?
There was a problem hiding this comment.
It's probably the arrays, but it would need figuring out. Keep in mind windows has a 1MB default, while Linux usually has 8MB. This brings it to 8MB.
There was a problem hiding this comment.
The test cases explicitly state that they may require quite absurd stack sizes. 48MB is huge. Not sure why you would ever need that. And clearly was an overestimation.
There was a problem hiding this comment.
This is really weird. Not sure I understand the reason for why it was done like this. The first thing that is done is allocating 1MB of random data on the stack. https://github.com/fortran-lang/stdlib/blob/master/test%2Fhashmaps%2Ftest_open_maps.f90#L32
While the comment was saying 48MB that was for values of rand_power above 18. rand_size is 2**rand_power. rand_object is an integer array of rand_size elements. So this immediately overflows the stack on windows.
There was a problem hiding this comment.
I'm not familiar with the test if indeed, as an unit test does it really require such a large array? moving the array to the heap using an allocatable seems like a good solution to avoid the stack overflow, so this specific hack to increase the stack size would be not required.
There was a problem hiding this comment.
Yep, I would be in favor of attempting to replace all arrays in the tests with allocatables and remove the stack flags.
There was a problem hiding this comment.
humm... if I revert the stack size flag I have several tests failing:
The following tests FAILED:
1 - test-drive/all-tests (Exit code 0xc0000374
)
12 - chaining_maps (SEGFAULT)
13 - open_maps (SEGFAULT)
14 - maps (SEGFAULT)
15 - intrinsics (Failed)
30 - linalg_pseudoinverse (Failed)
43 - sorting (Exit code 0xc0000374
)
47 - mean (Failed)
64 - string_to_number (Failed)
66 - filesystem (Failed)
71 - simps (Failed)
It seems like many of them are using stack variable declaration instead of heap variables.
There was a problem hiding this comment.
That is interesting. That is more fails than I had.
There was a problem hiding this comment.
That's on my personal laptop, it has a smaller memory compared to a proper workstation.
There was a problem hiding this comment.
I am testing on 32gb of memory. Also stack space is independent of main memory size.
|
Stack space is supposed to be faster, but is the differrnce in performance
really measurable? An alternative might be to combine them: use small
wrappers that pass either automatic or explicitly allocated arrays to the
actual routines that do the work. But a test to see the performance might
be useful.
Op za 3 mei 2025 06:41 schreef jalvesz ***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In test/CMakeLists.txt
<#988 (comment)>:
> @@ -2,6 +2,14 @@ if (NOT TARGET "test-drive::test-drive")
find_package("test-drive" REQUIRED)
endif()
+if(WIN32)
+ if(CMAKE_Fortran_COMPILER_ID MATCHES "^Intel")
+ add_link_options(/Qoption,link,/STACK:8388608)
That's on my personal laptop, it has a smaller memory compared to a proper
workstation.
—
Reply to this email directly, view it on GitHub
<#988 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN6YR46IQDLFV3NWLNI2AT24RJIHAVCNFSM6AAAAAB34BLDLSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMJTGE3DCNJVHE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Just making it an allocatable should be fine. Even just holding a smaller array and refilling it should work. Holding a big array of random values is a rather weird thing to do. |
|
Tried with the open maps test to make it allocatable but it's still overflowing the stack. |
This PR addresses #979 and #971 partially.
For intel compiler some dependencies with mvsc need to be managed.
Another problem is the stack memory size limit that needs to be increased for Windows.
cc @R-Goc @zoziha @jvdp1 @perazz